chore: bump OCCTSwift floor to 2.0.0, fix #763/#642 breaks - #112
Merged
Merged
Conversation
OCCTSwift v2.0.0 is a correctness major (17 breaking changes to the public
Swift API, OCCT absorbed to 8.0.1; docs/SEMVER.md#v200 in the OCCTSwift repo).
Audited every OCCTSwift call site in this repo against the full break table,
not just the issue's own first-pass grep, and fixed two real breaks:
1. ShapeAnalysisResult.selfIntersectionCount removed (#763; always 0, never
computed, no salvageable replacement value). Heal.swift and
GraphValidate.swift both derived a reported field straight from it
(Heal's `selfIntersectionsResolved` diff, GraphValidate's `selfIntersecting`
boolean), so both were already reporting a fabricated always-0/always-false
answer before this bump forced the compile error. Fixed by switching to the
real, opt-in check: `hasSelfIntersection: Bool?` / `selfIntersecting: Bool?`
via `Shape.analyze(selfIntersectionTimeout:)`, nil ("not checked") by
default since the check is ~3000x an ordinary scan on pathological input
and both verbs would run it twice. A new `--self-intersection-timeout`
flag opts in on both commands.
2. AAG builds nodes from face occurrences, not distinct faces (#642): AAG's
own node index (`AAGNode.faceIndex`, `PocketFeature.floorFaceIndex`/
`wallFaceIndices`, `detectHoles()`'s `faceIndex`, `AAGEdge.face1Index`/
`face2Index`) now indexes `Shape.orientedFaces()`, an occurrence index,
not `Shape.faces()`'s deduplicated `face[N]` scheme `query-topology`
emits. The two agreed automatically pre-2.0.0 (faces() was itself
occurrence-based then), so three commands that explicitly document
aligning their own face-index output with query-topology's `face[N]`
(FeatureRecognize.swift, both the occtkit command and the legacy
standalone target, GraphSelect.swift, and GraphML.swift) silently
started naming the wrong face (or, for graph-ml, dangling-referencing
past its own faces[] array) on any multi-solid compound with a shared
face. Fixed by resolving through the new `AAGNode.distinctFaceIndex`
bridge everywhere AAG output crosses into a face[N]-shaped response; a
no-op on any shape that shares no face (every single-solid part), which
is why this repo's pre-existing tests never exercised it.
`graph-select`'s `face-neighbors` query additionally gained a `warning`
field for the case where `--face` names a shared face and the response
can only speak for the first occurrence.
Added Tests/OcctkitCommandTests/AAGFaceIndexTests.swift: a real regression
suite against a split-box-compound fixture (matching OCCTSwift's own
orientedFaces()/distinctFaceIndex doc example, 11 distinct faces over 12
occurrences), driving GraphSelectCommand/GraphMLCommand directly via
@testable import and asserting on their actual JSON output. Verified each
assertion fails if the fix is reverted (not just that it passes). Both new
and existing suites needed a `.serialized` trait: running OCCTSwift Shape
construction across concurrent Swift Testing suites produced an intermittent
SIGSEGV under the default parallel scheduler, a pre-existing OCCT
thread-safety gap this repo's test suite hadn't exercised until now (it only
had one suite before).
Audited clean (no code change needed) against the rest of the v2.0.0 break
table: the sub-shape-enumeration family (#541/#568/#613, this repo either
enumerates via `.faces()`/`.edges()`/`.vertices()` directly and inherits the
corrected count/order automatically, or via `subShapeCount(ofType:)`/
`subShapes(ofType:)`, already map-backed and unaffected), #502
(`Shape.wires`/`.shells`/`.solids`, not used here), the mass-property surface
(#605/#609, `.volume`/`.centerOfMass` etc. were already Optional pre-2.0.0
and every call site already propagates nil), `VinertGKResult.absoluteError`/
`BisectorPoint`/`PathParser`/`continuityOrder`/`buildCurves3d` (no call sites
at all).
Docs updated in every place they documented the removed/changed fields:
docs/reference/{engineering,topology-graph,introspection,occtkit-verbs}.md,
docs/guides/cookbook/{engineering-analysis,topology-graph,
introspection-and-measurement}.md, README.md, CLAUDE.md.
Known blocker, not fixable from this repo: the rest of the cohort has not
yet released a version compatible with OCCTSwift 2.0.0. Confirmed by a real
`swift package resolve` against a fresh checkout with no local sibling:
OCCTSwiftIO's latest release (v1.7.6) still requires
`occtswift 1.17.0..<2.0.0` transitively, and OCCTSwiftTools/AIS/Mesh's own
latest releases are all still on their own "repin to 1.17.0" floors.
`swift build`/`swift test` are clean here via the local-sibling-checkout
trick (every sibling's own not-yet-released local checkout has already
moved its floor to 2.0.0 too), but a fresh clone / CI run cannot resolve the
graph from remote until at least OCCTSwiftIO ships a compatible release.
Recorded as an OKF decision (okf/decisions/occtswift-2.0.0-floor-bump-blocked-on-cohort-releases.md)
per this repo's PR template.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The current output.brep's volume (8575.154) no longer matches what Shape.pipeShell(solid: true) on a helix spine produces under OCCTSwift 2.0.0 (9765.436, a 14% drift past this recipe's own 0.1% tolerance) — filed as SecondMouseAU/OCCTSwift#830, looks like a genuine kernel-level regression (likely coil self-overlap), not a bug in this recipe or this repo. Archiving the old reference under this name rather than deleting it outright, so whoever fixes #830 has the known-good comparison point to regenerate against.
…as output.brep.pre-2.0.0-reference-see-issue-830)
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
OCCTSwift v2.0.0 is a correctness major (17 breaking changes to the public Swift API, OCCT absorbed to 8.0.1;
docs/SEMVER.md#v200in the OCCTSwift repo). This bumps the floor pin and fixes every break that actually needed a source change.Confirmed compile break, fixed:
ShapeAnalysisResult.selfIntersectionCountwas removed (#763; always0, never computed, no salvageable replacement value).Heal.swiftandGraphValidate.swiftboth derived a reported field straight from it, so both were already reporting a fabricated always-0/always-falseanswer before this bump forced the compile error. Both now report the real, opt-in check (hasSelfIntersection/selfIntersecting: Bool?viaShape.analyze(selfIntersectionTimeout:)),nil("not checked") by default since the check is roughly 3000x an ordinary scan on pathological input and both verbs would run it twice.Real bug found by the full audit, not in the issue's own first-pass grep: AAG builds nodes from face occurrences, not distinct faces (#642).
AAGNode.faceIndex,PocketFeature.floorFaceIndex/wallFaceIndices,detectHoles()'sfaceIndex, andAAGEdge.face1Index/face2Indexnow indexShape.orientedFaces()(an occurrence index), notShape.faces()'s deduplicatedface[N]schemequery-topologyemits. The two agreed automatically pre-2.0.0 (faces()was itself occurrence-based then), so three commands that explicitly document aligning their own face-index output withquery-topology'sface[N](FeatureRecognize.swift, both theocctkitcommand and the legacy standalone target,GraphSelect.swift, andGraphML.swift) silently started naming the wrong face, or forgraph-ml, dangling-referencing past its ownfaces[]array, on any multi-solid compound with a shared face. Fixed by resolving through the newAAGNode.distinctFaceIndexbridge everywhere AAG output crosses into aface[N]-shaped response.graph-select'sface-neighborsquery also gained awarningfield for the one remaining case that needs disclosure:--facenaming a face shared between two solids, where the response can only speak for the first occurrence.Audited clean (no code change needed) against the rest of the v2.0.0 break table: the sub-shape-enumeration family (#541/#568/#613), #502, the mass-property surface (#605/#609), and
VinertGKResult/BisectorPoint/PathParser/continuityOrder/buildCurves3d(no call sites, or already using the map-backed/Optional-returning APIs the break table describes as the target state).Docs updated everywhere they documented the removed/changed fields, plus an OKF decision record for the cross-repo blocker below.
Checklist
Tests/OcctkitCommandTests/AAGFaceIndexTests.swiftis a real regression suite against a split-box-compound fixture (matching OCCTSwift's ownorientedFaces()/distinctFaceIndexdoc example), drivingGraphSelectCommand/GraphMLCommanddirectly via@testable importand asserting on their actual JSON output. Verified each assertion fails if the fix is reverted, not just that it passes.Notes for the reviewer
Addresses #111, does not close it. Checklist status against the issue's own list:
selfIntersectionCountbreakdocs/SEMVER.md#v200(found and fixed the #642 AAG issue above, beyond the issue's own first-pass grep)2.0.0swift build && swift testclean, locally, via the sibling-checkout trickswift package resolveagainst a copy of this repo with no local sibling checkout fails, because OCCTSwiftIO's latest release (v1.7.6) still requiresocctswift 1.17.0..<2.0.0transitively, and OCCTSwiftTools/AIS/Mesh's own latest releases are all on the same "repin to 1.17.0" floor. This is wider than just Tools. Seeokf/decisions/occtswift-2.0.0-floor-bump-blocked-on-cohort-releases.mdfor the full writeup and the exact resolver error. Per this repo's own repo-boundary convention, no release/tag is cut here; that is for the maintainer once the cohort has caught up.Two things found and fixed along the way that are worth flagging explicitly since they are not part of the OCCTSwift break itself:
Shapeconstruction across two concurrent Swift Testing suites produced an intermittent SIGSEGV under the default parallel scheduler. Both the new suite and the existingAssemblyComposerTestssuite now carry a.serializedtrait, which was verified empirically to eliminate it over 15+ repeated runs (this repo only had one test suite before, so nothing had exercised cross-suite concurrent OCCT calls until now).Package.resolvedis intentionally left untouched, matching the precedent set byd5d31e8(a prior pure floor-bump commit that also touched onlyPackage.swift): the local build resolves via the sibling-checkout trick, which does not produce a meaningfulPackage.resolvedupdate, and a real remote resolution is not possible yet for the reason above.